[typescript] [createMuiTheme] Fix typings & deepmerge shape #11993
Conversation
| @@ -25,6 +25,7 @@ export interface ThemeOptions { | |||
| transitions?: TransitionsOptions; | |||
| typography?: TypographyOptions | ((palette: Palette) => TypographyOptions); | |||
| zIndex?: ZIndexOptions; | |||
| status?: Record<string, string>; | |||
There was a problem hiding this comment.
No, it's too specific.
| @@ -13,7 +13,7 @@ import { ComponentsProps } from './props'; | |||
| export type Direction = 'ltr' | 'rtl'; | |||
|
|
|||
| export interface ThemeOptions { | |||
| shape?: ShapeOptions; | |||
| shape?: Shape; | |||
There was a problem hiding this comment.
Don't we still need the partial? Like we do for the Spacing.
There was a problem hiding this comment.
Unlike spacing, which is deepmerged, shape is not.
https://github.com/mui-org/material-ui/blob/ca33d9773581a8b0ff37c92260b48c9f72b42de9/packages/material-ui/src/styles/createMuiTheme.js#L38-L44
This means that for spacing, we can specify just partial of it, but for shadow, we need to specify the full props, from what I understand.
There was a problem hiding this comment.
FYI - if you take a look at shadow type definition under the ThemeOptions interface, it is not Partial<> either.
https://github.com/mui-org/material-ui/blob/ca33d9773581a8b0ff37c92260b48c9f72b42de9/packages/material-ui/src/styles/createMuiTheme.d.ts#L23
There was a problem hiding this comment.
Thanks for raising the issue! Should we fix the implementation over the TypeScript definition then? I have missed this part. I think that spacing and shape should behave the same way.
There was a problem hiding this comment.
I see. I'll do this. BTW, what about shadows? Should it also be a partial in the ThemeOptions?
There was a problem hiding this comment.
we need to specify the full props
I don't think that it's the behavior people expect. We need to fix the shape key :).
There was a problem hiding this comment.
NVM. shadows is different.
There was a problem hiding this comment.
Regarding the shadows, yeah it's fine. Relative values are important, people need to provide the full property.
| @@ -85,6 +88,4 @@ export interface PaletteOptions { | |||
| getContrastText?: (background: string) => string; | |||
| } | |||
|
|
|||
| //export type PaletteOptions = DeepPartial<Palette>; | |||
There was a problem hiding this comment.
This was commented out. Why do we want to preserve the commented out code?
There was a problem hiding this comment.
I was wondering why it was commented out; Let's remove it then :)
| @@ -40,6 +41,7 @@ export interface Theme { | |||
| transitions: Transitions; | |||
| typography: Typography; | |||
| zIndex: ZIndex; | |||
| status?: Record<string, string>; | |||
There was a problem hiding this comment.
No, it's too specific.
franklixuefei
changed the title
|
@franklixuefei So many bug fixes in a single pull request 🎉! |
|
@oliviertassinari Thanks for taking care of this! I actually left out those two props |
* fix typings * fix * revert * fix shape * fix unit test * add unit test
fixes #11853